-
Notifications
You must be signed in to change notification settings - Fork 193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
internal/devpkg: add PackageSpec for parsing package strings #1878
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be really nice to have. Take a look at comments before we settle on parsing implementation.
// | Syntax | Example | Description | | ||
// | ---------------- | --------------------------- | ------------------------------------ | | ||
// | name@version | go@v1.22 | Resolved w/ search service | | ||
// | name | go | Same as above; implies @latest | | ||
// | flake | github:/nixos/nixpkgs#go | Matches Nix installable syntax | | ||
// | attr path | go | Equivalent to flake:nixpkgs#go | | ||
// | legacy attr path | go | Uses deprecated nixpkgs.commit field | | ||
// | runx | runx:golangci/golangci-lint | Experimental | | ||
// | ||
// Most package specs are ambiguous, making it impossible to tell which exact | ||
// syntax the user intended. PackageSpec parses as many as it can and leaves it | ||
// up to the caller to prioritize and resolve them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should understand a string that has no version and is not a flake/runx to always mean attribute path. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tricky part is that almost any string is a valid flake reference due to indirect flakes. Still, I’d be onboard with saying we just don’t support scheme-less indirect flakes. So whereas Nix does:
foo → flake:foo
flake:foo → flake:foo
Devbox would do:
foo → flake:nixpkgs#foo
flake:foo → flake:foo
That still leaves ambiguity between Devbox packages and attribute paths though. We can't tell if nodePackages.@angular/cli
is named nodePackages.@angular/cli
or named nodePackages.
with version angular/cli
without querying the search service.
// | Syntax | Example | Description | | ||
// | ---------------- | --------------------------- | ------------------------------------ | | ||
// | name@version | go@v1.22 | Resolved w/ search service | | ||
// | name | go | Same as above; implies @latest | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not really valid. (or should not be). I was under impression that anything without a version is understood to mean:
nixpkgs/<hash>#attr
When someone goes devbox add go
it adds go@latest
to devbox.json.
If someone does devbox add stdenv.cc.cc.lib
which is not in the index, then it should add stdenv.cc.cc.lib
which represents nixpkgs/<hash>#stdenv.cc.cc.lib
If someone manually edits the devbox.json to add attribute paths without @version
flag they get nixpkgs/<hash>#attr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also support nixpkgs attribute path via nixpkgs#hello
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, I didn't realize that devbox add go
and manually adding go
to devbox.json
did different things. I never would've guessed that {"packages": ["go"]}
gives me an old version of Go, especially because there's no nixpkgs.commit
field.
Could we align the CLI and config to both follow the CLI's behavior of checking for an @latest version? Having the same package string mean different things depending on if it's given on the command line vs. a config is pretty confusing IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how often that @latest
user is accidentally mentioned on GitHub.
// | name | go | Same as above; implies @latest | | ||
// | flake | github:/nixos/nixpkgs#go | Matches Nix installable syntax | | ||
// | attr path | go | Equivalent to flake:nixpkgs#go | | ||
// | legacy attr path | go | Uses deprecated nixpkgs.commit field | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it deprecated? We removed it because we don't really want anyone to use it. I guess our preference would be for folks to use the hash in the package they want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, our docs say it's deprecated and will eventually be removed. I think pinning using a revision in a flake reference is clearer.
// For example, the package spec "cachix" is a Devbox package (implied @latest), | ||
// a flake ref (cachix), and an attribute path (nixpkgs#cachix). When resolving | ||
// the package, Devbox must pick one (most likely cachix@latest). | ||
type PackageSpec struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit should this go in own file?
Attempt to formalize raw Devbox package strings (hereby called package specs) by introducing a new `PackageSpec` type. This commit just adds the new parsing logic, it doesn't integrate it with any other code yet. The `PackageSpec` type is the result of parsing a raw string with `ParsePackageSpec`: type PackageSpec struct { Name, Version string Installable flake.Installable AttrPathInstallable flake.Installable RunX types.PkgRef } func ParsePackageSpec(raw, nixpkgsCommit string) PackageSpec When a package spec is ambiguous, `ParsePackageSpec` will populate the `PackageSpec` fields with all possible interpretations. For example, the spec `go` could be `go@latest`, a flake named `go`, or the attribute path `nixpkgs#go`. It's up to a resolver to try them in some priority order and pick one.
8f8cc3d
to
58c57a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just commenting (some old comments too) to remove this PR from my review queue
@@ -366,3 +368,134 @@ func packagesFromLegacyList(packages []string) []Package { | |||
} | |||
return packagesList | |||
} | |||
|
|||
// PackageSpec specifies a Devbox package to install. Devbox supports a number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to own file?
// | ||
// | Syntax | Example | Description | | ||
// | ---------------- | --------------------------- | ------------------------------------ | | ||
// | name@version | go@v1.22 | Resolved w/ search service | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be go@1.22
?
I did not think we support the v1.22
syntax. Do we?
// syntax the user intended. PackageSpec parses as many as it can and leaves it | ||
// up to the caller to prioritize and resolve them. | ||
// | ||
// For example, the package spec "cachix" is a Devbox package (implied @latest), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we pick a different example package? :)
Attempt to formalize raw Devbox package strings (hereby called package specs) by introducing a new
PackageSpec
type.This commit just adds the new parsing logic, it doesn't integrate it with any other code yet. The
PackageSpec
type is the result of parsing a raw string withParsePackageSpec
:When a package spec is ambiguous,
ParsePackageSpec
will populate thePackageSpec
fields with all possible interpretations. For example, the specgo
could bego@latest
, a flake namedgo
, or the attribute pathnixpkgs#go
. It's up to a resolver to try them in some priority order and pick one.